Skip to content

Conversation

@MichaelChirico
Copy link
Collaborator

@MichaelChirico MichaelChirico commented May 27, 2022

Related: #1292

@AshesITR
Copy link
Collaborator

I noticed .dev isn't covered by our lintr suite. Do we want to do something about it?

@MichaelChirico
Copy link
Collaborator Author

I guess we should also include a recommendation to add us as Suggests in the README? there's typically CRAN requirement all but it'll make things easier

@MichaelChirico
Copy link
Collaborator Author

I noticed .dev isn't covered by our lintr suite. Do we want to do something about it?

i think it's fine... not really production scripts, fine if they're a little rough around the edges

@MichaelChirico
Copy link
Collaborator Author

MichaelChirico commented Jun 5, 2022

@AshesITR PTAL. Added the e-mail templates -- these again will not be stored on GH (though the revdep-email-template file will be).

We can fine-tune the output a bit (or handle any bigger feedback), then I'll remove the temp artefacts & we can merge.

After that I'll put out a call for new untracked repos. I prefer if downstreams declare themselves to us, btw, rather than getting in downstreams' hair uninvited and/or dealing with low-quality/unmaintained repos. It's also a bit more of a security risk.

@AshesITR
Copy link
Collaborator

AshesITR commented Jun 6, 2022

I took a glimpse now.

  • Some maintainers might get multiple mails. Do we want to bundle all info in one mail?
  • The lint comparisons for brace_linter() and its predecessors always show up as a change, but I don't think this is material. Can we somehow exclude these sporadic hits?
  • Would it be worthwhile to dynamically list what is attached? e.g. if a failure is detected.
  • For the failures pertaining to a removed deprecated function, we could repeat the new linter to ease transition after an update to 3.0.0 has already been made. I see absolute_paths_linter() causing a few failures.

Also, maybe a good idea to run the checks with a soft-deprecated shim for the removed linters so we can check the remainder of the packages using now removed linters.
IINM we can achieve this by supplying the removed linter functions in globalenv() before calling lint_package().

@MichaelChirico
Copy link
Collaborator Author

Some maintainers might get multiple mails. Do we want to bundle all info in one mail?

Do you know what revdepcheck does here? i can see benefits of both ways. currently there are at most a few e-mails to be received so it may not be worth over-engineering...

The lint comparisons for brace_linter() and its predecessors always show up as a change, but I don't think this is material. Can we somehow exclude these sporadic hits?

Note that the differences are not matching on linter name, just on package/filename/line_number. So if brace_linter() is showing up, either the source marker has moved, or it's a new/removed lint. In any case, it should be surfaced to make sure it's intentional.

Would it be worthwhile to dynamically list what is attached? e.g. if a failure is detected.

Not following what this would look like...

For the failures pertaining to a removed deprecated function, we could repeat the new linter to ease transition after an update to 3.0.0 has already been made. I see absolute_paths_linter() causing a few failures.

I think there's been transition enough -- as noted in the NEWS, they've been marked as deprecated already since 2017. Time to pull the plug.

Also, maybe a good idea to run the checks with a soft-deprecated shim for the removed linters so we can check the remainder of the packages using now removed linters. IINM we can achieve this by supplying the removed linter functions in globalenv() before calling lint_package().

great idea 👍

@MichaelChirico
Copy link
Collaborator Author

OK @AshesITR cleaned up the PR here, should be ready for merge if there are no blockers

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, thanks!

@MichaelChirico MichaelChirico requested a review from AshesITR June 13, 2022 07:45
@AshesITR AshesITR merged commit 3eb998e into main Jun 13, 2022
@AshesITR AshesITR deleted the rev-urls branch June 13, 2022 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants